Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: eip target gas from consensus params #7270

Merged
merged 12 commits into from
Jan 9, 2024
Merged

feat: eip target gas from consensus params #7270

merged 12 commits into from
Jan 9, 2024

Conversation

czarcas7ic
Copy link
Member

@czarcas7ic czarcas7ic commented Jan 8, 2024 •
edited
Loading

Closes: #XXX

What is the purpose of the change

The EIP TargetGas is hardcoded as a variable to 75_000_000.

This PR does two things:

  1. Sets the TargetGas to whatever the consensus param's MaxGas for a block
  2. Caches this value and fetches the consensus param bytes every block. If the bytes change, we unmarshal and set the TargetGas to the new value.

This allows us to utilize this parameter without having to deserialize the value every block.

Testing and Verifying

BeginBlocker test added here f6832ce

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

@czarcas7ic czarcas7ic added the V:state/breaking State machine breaking PR label Jan 8, 2024
@github-actions github-actions bot added C:x/epochs C:x/txfees C:app-wiring Changes to the app folder labels Jan 8, 2024
ValarDragon
ValarDragon reviewed Jan 8, 2024
View reviewed changes
x/txfees/module.go
}

newBlockMaxGas := newConsensusParams.Block.MaxGas
mempool1559.TargetGas = newBlockMaxGas
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't right, it should be max gas times .625

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done here d148048

@czarcas7ic czarcas7ic marked this pull request as ready for review January 8, 2024 23:59
@czarcas7ic czarcas7ic marked this pull request as draft January 9, 2024 01:32
@czarcas7ic czarcas7ic marked this pull request as ready for review January 9, 2024 02:14
ValarDragon
ValarDragon approved these changes Jan 9, 2024
View reviewed changes
Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job!

@czarcas7ic czarcas7ic merged commit 875a12a into main Jan 9, 2024
1 check passed
@czarcas7ic czarcas7ic deleted the adam/eip-target-gas-var branch January 9, 2024 19:17
@github-actions github-actions bot mentioned this pull request Apr 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PaddyMc PaddyMc PaddyMc approved these changes

@ValarDragon ValarDragon ValarDragon approved these changes

@nicolaslara nicolaslara Awaiting requested review from nicolaslara nicolaslara is a code owner

@p0mvn p0mvn Awaiting requested review from p0mvn p0mvn is a code owner

@AlpinYukseloglu AlpinYukseloglu Awaiting requested review from AlpinYukseloglu AlpinYukseloglu is a code owner

@mattverse mattverse Awaiting requested review from mattverse mattverse is a code owner

Assignees
No one assigned
Labels
C:app-wiring Changes to the app folder C:x/epochs C:x/txfees V:state/breaking State machine breaking PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@czarcas7ic @ValarDragon @PaddyMc